Skip to content

Quote Select, join and GROUP BY column names as identifiers - #241

Open
simon-mundy wants to merge 6 commits into
php-db:0.6.xfrom
simon-mundy:fix/select-column-identifier
Open

simon-mundy wants to merge 6 commits into
php-db:0.6.xfrom
simon-mundy:fix/select-column-identifier

Conversation

@simon-mundy

@simon-mundy simon-mundy commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Merge order: merge this PR before #243 and #244. Both build on the IdentifierQuoter added here, so until this PR is merged their diffs also show its changes.

Problem

Select fragment-quotes column names that are not plain or dotted words. On the SQL-92 platform:

Column string Rendered before Rendered now
x-y "foo"."x""-""y" AS "x-y" "foo"."x-y" AS "x-y"
my col "foo"."my" "col" AS "my col" "foo"."my col" AS "my col"
a"b "foo"."a""'""b" AS "a'b" "foo"."a'b" AS "a'b"

The alias was already quoted as a whole name, so the column and its alias disagreed. Join column lists and GROUP BY render the same broken way.

Cause

processSelect() (select and join columns) and processGroup() resolve each column string through AbstractSql::resolveColumnValue() with isIdentifier. That ends in AbstractPlatform::quoteIdentifierInFragment(), which splits the string on every character outside the platform's word pattern and quotes each piece. Only *, space, . and as are left bare (SAFE_WORDS). ORDER BY names (#243), predicate names (#244) and Update SET columns (#238) have the same cause.

Fix: one shared IdentifierQuoter

This PR adds PhpDb\Sql\IdentifierQuoter (@internal), which #243 and #244 reuse, so every identifier slot follows one rule. Select overrides resolveColumnValue() and passes an identifier column string to IdentifierQuoter::quoteColumn().

  1. Fast path. A plain name of word characters and qualifier dots, optionally ending in .* (id, t.a, t.*), goes straight to quoteIdentifierInFragment(). That renders it exactly as before and skips the rest of the check, so the common case stays cheap.
  2. Otherwise IdentifierShape reads the name in pieces between spaces, dots and *, and IdentifierPiece classifies each piece by asking the platform itself:
    • Fragment: fragment quoting renders it as written, so it is kept, byte-identical to today. Examples: price*qty, a as b, t.a * t.b.
    • Whole: a name that fragment quoting would split (x-y, my col, café, t.as) is quoted whole. It is split only on ., each part is trimmed and passed through quoteIdentifier(), a * part stays bare, and the parts are joined with getIdentifierSeparator().
    • Unquotable: expression syntax that no identifier quoting renders as written throws InvalidArgumentException::forUnquotableIdentifier(), pointing to Expression. This covers a bare operator (a - b), a function call (COUNT(id)), and a whole-quoted name mixed with * or AS (x-y * 2).
    • Empty part: an empty name or an empty qualifier part ('', a..b, t.) throws InvalidArgumentException::forEmptyIdentifierPart().
  3. Inline alias. In a column, x-y as z renders as "x-y" AS "z", with each side quoted separately.

The pieces are classified with the platform's own quoteIdentifierInFragment() and quoteIdentifier(), so each platform's fragment pattern is respected. phpdb-mysql treats - as a word character, so `x-y` and `a-b` * `c` render as before there. With quoteIdentifiers off, both methods return the string as given, so nothing changes.

Expressions, sub-selects and every other use of resolveColumnValue() (Update, Insert) still go through the parent method unchanged.

What stays the same

docs/book, the unit tests, TableGateway/RowGateway and phpdb-mysql, phpdb-sqlite and phpdb-pgsql only pass plain names, dotted names, * and aliased keys as column strings. SQL always goes through Expression. All of these render byte-for-byte as before, as does every string that renders valid SQL today: t.*, price * qty, a*b, t.price * t.qty, and the inline alias bar AS baz.

Behaviour change

Output changes only for strings that rendered invalid SQL before:

  • a hyphen, space, quote character or non-ASCII letter in a name now renders a valid quoted name;
  • expression syntax mixed into an identifier string (COUNT(id), x-y * 2), and an empty name or qualifier part, now throw InvalidArgumentException. They used to render broken SQL, which only failed at the database.

Performance

Measured locally with php -n, rendering a 15-identifier Select, best of 5 runs:

Time per render
0.6.x 48.6 µs
this PR 52.5 µs (+8%)
this PR + #243 + #244 54.0 µs (+12%)

The previous per-PR helpers cost +35% here, and +58% combined. The fast path accounts for most of the difference.

Tests

  • test/unit/Sql/IdentifierQuoterTest.php (#[Group('unit')]): 48 data-provider cases.
    • names quoted as written, in every shape above, including padded and qualified parts and t.as;
    • unquotable expressions and empty parts, each asserting its exception;
    • inline column aliases;
    • a hyphen-as-word platform (the shared TrustingHyphenWordPlatform asset), identifier quoting off, and the platform's identifier separator;
    • the fast path itself.
  • test/unit/Sql/SelectColumnNameTest.php: select columns, join columns and GROUP BY, including rejected expressions in each.

Checks:

  • Infection kills every mutant in IdentifierQuoter, IdentifierShape and IdentifierPiece.
  • The full suite passes.
  • mago format --check, mago lint and mago analyze exit 0 on PHP 8.3, 8.4 and 8.5, and so does mago guard.

Follow-ups (not in this PR)

  • ORDER BY string parsing: 'my col DESC' parses as the name my with an unknown direction ("my" ASC). This is a parsing problem, not a quoting one.
  • DDL names (column, index and constraint names) should always be quoted whole, since they can never be expressions.
  • Qualified star alias: an unaliased t.* in the select or join list still gets AS "t.*" appended.
  • Inline AS in join columns: the join list appends a second alias ("zac"."bar" AS "baz" AS "bar AS baz").
  • Platform quoting (from Quote Update SET columns as identifiers and bind them under valid placeholder names #238): SQL-92 and SQLite use ' for quoteIdentifierTo, so an embedded " becomes ' rather than "".

Select resolved each column string through quoteIdentifierInFragment(),
which splits on every non-word character, so a column such as x-y
rendered as "x""-""y" while its alias rendered as "x-y". Column strings
in the select list, join column lists and GROUP BY are now quoted as an
identifier chain split only on the table-qualifier dot, with a * part
left bare. A string holding an inline " AS " alias keeps fragment
quoting, as before.
@codecov

codecov Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

A column string now switches to identifier-chain quoting only when
fragment quoting would render it broken: a piece between the safe
delimiters (space, "." and "*") that the platform would split and quote
in parts, as with x-y, a"b or COUNT(id), or two names separated by
spaces alone, as with "my col". Strings such as "price * qty" or "a*b"
render exactly as before.
simon-mundy and others added 2 commits November 4, 2026 14:26
Replace Select's own column-quoting helpers with IdentifierQuoter, which
the ORDER BY and predicate fixes can share:

- A plain name of word characters and qualifier dots takes a fast path
  straight to fragment quoting, as before.
- Otherwise IdentifierShape reads the name in pieces. Fragment quoting is
  kept where it renders as written; a name it would split (x-y, my col,
  t.as) is quoted whole per qualifier part, with each part trimmed.
- Expression syntax that no identifier quoting renders as written, such
  as COUNT(id), a - b or x-y * 2, throws InvalidArgumentException, as
  does an empty name or qualifier part.
- An inline column alias such as 'x-y as z' renders "x-y" AS "z".

TrustingHyphenWordPlatform becomes the shared hyphen-as-word test
platform.
simon-mundy added a commit to simon-mundy/phpdb that referenced this pull request Nov 4, 2026
simon-mundy added a commit to simon-mundy/phpdb that referenced this pull request Nov 4, 2026
The test restricts coverage to the factories it lists, so the new
forEmptyIdentifierPart() and forUnquotableIdentifier() counted as
uncovered and failed codecov/patch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant